rfc: Change Electron Fuse Security Defaults#7
Conversation
|
|
||
| ## Unresolved questions | ||
|
|
||
| N/A |
There was a problem hiding this comment.
Rewording from a Slack message I made in #wg-releases back in February when these changes were first discussed. I've been intending to create an RFC describing this possible implementation, but haven't had the time. I'd still like to raise these concerns:
Historically the defaults for fuses have been permissive, and they’ve been flipped to more restrictive at packaging time. With the discussed changing of these defaults, this will no longer be true, and I don’t think we have a good DX for how app developers flip it back to true in development if their app needs that functionality.
Additionally, some fuses make sense to only flip at packaging time (onlyLoadAppFromAsar, nodeCliInspect, etc) but I think some would be beneficial to also flip in dev mode so that you have better consistency between dev and prod (runAsNode, grantFileProtocolExtraPrivileges, etc) and don’t get surprised by behavior differences.
Basically, I think it should be more straight forward for app developers to flip fuses in development. It’s technically possible at the moment by rolling your own using the @electron/fuses CLI, but it would be awkward DX and I don’t think we should push that to app developers to figure out if we could just make it supported functionality.
One possible implementation, for the sake of further illustration (would be fleshed out in a separate RFC):
- Conceptually support a
config.@electron/fuseskey inpackage.jsonwhich defines boolean values for fuses, similar to how we haveconfig.forgefor Forge - Extend
@electron/fusesCLI to add a new command (and expose it programmatically as well), say@electron/fuses applywhich would read the config out of your app’spackage.jsonand apply it to the Electron executable innode_modules- On first run it would create
node_modules/electron/original_fuses.json(similar to how we dump the executable path intopath.txt) so that the default state for any given fuse can always be restored by the CLI
- On first run it would create
- Update
npm/cli.jsin thee/erepo so that before spawning Electron we apply fuses from the config (or reset them back to defaults if the developer has removed the config)
We could then also leverage config.@electron/fuses in Fiddle to support flipping fuses there. When uploading a fiddle to a Gist it would include that config and Fiddle could read it back on load and flip the fuses accordingly (requires some @electron/fiddle-core changes to ensure flipped fuses don’t bleed between runs).
|
I'm generally in favor of this RFC as switching Electron to secure by default settings is a good idea. We should also consider discussing @dsanders11's feedback in an API WG meeting. @MarshallOfSound is there still interest in pushing forward this effort? If so, would you like to schedule a discussion for it next month? |
|
Due to the lack of an owner actively pushing this RFC forward, I'll be closing this PR. If there's interest in revisiting this RFC, reopening or creating a new PR is always welcome and encouraged! |
|
Hey folks, I'd like to reopen this and continue moving this forward - happy to assign myself as owner for the actual RFC process, I'll reopen this PR and push updates to it for timelines specifically |
|
Per conversation in the API working group meeting, we discussed having defaults for packaged apps vs the dev runner. Here is my branch that adds those. The actual defaults we'll use will continue to be discussed. https://github.com/nmggithub/electron/tree/better-fuse-defaults |
itsananderson
left a comment
There was a problem hiding this comment.
Since we're talking about adding more states besides "on" and "off" (which would require a breaking schema change), I really think we should consider using using protocol buffers.
- They efficiently serialize to binary
- There's good tooling for generating TS and C++ serializers/deserializers from a
.protofile, so it'll be easy for both the app runtime and thefusesmodule to find the sentinel value and then deserialize the binary data. - They provide a good solution for backward compatibility, so when we add new fuses in future, or even if we want to add new "state" types, there will be less need to keep
electronand@electron/fusesin lockstep with each other.
|
|
||
| The fuses that should have their defaults changed are listed below, along with the theoretical impact of the change, and proposed timelines. | ||
|
|
||
| ### [`runAsNode`](https://www.electronjs.org/docs/latest/tutorial/fuses#runasnode) |
There was a problem hiding this comment.
I think runAsNode is useful for running unit tests. Should we have this one behave differently for packaged/unpackaged like nodeCliInspect?
I don't think we're adding enough state that's complex enough to require a full object model, but I understand your point. I'm attempting to maintain compatibility without a breaking change. In the future, Fuses might look differently enough to where we would benefit from something like Protobuf, but I'm not sure if we're there yet. |
Doesn't your proposed schema change require a version bump, thus causing a breaking change? |
The line says "in any breaking way" (emphasis mine). My change is not breaking. The semantics of the original |
|
The If we don't bump the schema version, it would also be possible for people to inadvertently set the new |
I appreciate the concern, and it's understandable. However, the current logic of Fuses is quite clear. If the value is not https://github.com/electron/electron/blob/v41.1.1/build/fuses/build.py#L86 It wouldn't be "undefined behavior", but your concerns may be valid if such behavior is undesirable. I do think it's ok behavior, as "off" should be a safe fallback. But others can feel free to disagree. |
|
You're right that "undefined" is perhaps hyperbolic, but maybe a better way to put it is "could cause behavior that is unexpected to the developer, and potentially dangerous". Some fuses are less secure when off. From a quick scan, these stand out to me:
If a developer tried to set any of these to |
This is a fair point. I think this should be discussed more. |
I think this is a pretty-simple fix. We just don't support people setting the value to The API as it exists should cover this: flipFuses({
version: V1,
[NodeFuse]: true
})Any non-referenced fuse remains at it's in-fuse value. i.e. in new electron it remains If someone wants to write it back to |
I think we wanted the "on only when packaged" and "on only in the dev runner" options to be user-configurable, did we not? |
We've talked about this a bit internally, but I think it would be good to formalize this as a proposal. This is less API discussion and more "proposing a breaking change" but I still felt it fits the RFC process a bit better than discussing in a PR somewhere.